Make --use-system-ca per-env rather than per-process #60678
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Aditi-1400
force-pushed
the
ca-per-env
branch
2 times, most recently
from
d87558c to
23473b9
Compare
|
Hmm, I think this may need more work than just updating the options - the implication of being per-env is that each worker would then be able to toggle this independently. Say when the main thread does not enable it but a worker does, then the worker will have the system CA certs in their default store but the parent doesn't. Can you add a test for this, and the other way around (parent enables it, worker disables it)? My impression is that the default store initialisation code is not yet ready for this and it's still shared across the process (so if a worker enables it, suddenly the parent get it too, which would be unexpected). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60678 +/- ##
==========================================
- Coverage 89.78% 89.74% -0.04%
==========================================
Files 673 673
Lines 203775 203853 +78
Branches 39166 39178 +12
==========================================
Hits 182956 182956
- Misses 13139 13199 +60
- Partials 7680 7698 +18
🚀 New features to boost your workflow:
|
Aditi-1400
force-pushed
the
ca-per-env
branch
2 times, most recently
from
f22457c to
780c272
Compare
Aditi-1400
force-pushed
the
ca-per-env
branch
4 times, most recently
from
38913f9 to
d9fb49d
Compare
There was a problem hiding this comment.
Can you add some tests to test/parallel that checks this affects tls.getCACertificates('default') in a worker if tls.getCACertificates('system') returns non-empty in a parent? (if there are no system CAs in the testing machine, then the test can be skipped - that way it can run even without the mock certificates installed)
Aditi-1400
force-pushed
the
ca-per-env
branch
2 times, most recently
from
77856be to
d8281c4
Compare
test/parallel/test-tls-get-ca-certificates-worker-use-system-ca.js
Outdated
Show resolved
Hide resolved
test/parallel/test-tls-get-ca-certificates-worker-use-system-ca.js
Outdated
Show resolved
Hide resolved
Aditi-1400
force-pushed
the
ca-per-env
branch
from
d8281c4 to
5d7dc6b
Compare
| } | ||
|
|
||
| Environment* env = Environment::GetCurrent(args); | ||
| const bool use_system_ca = env != nullptr && env->options()->use_system_ca; |
There was a problem hiding this comment.
Let's add a bit of log here to check whether it's hit on the env == nullptr path.
| const bool use_system_ca = env != nullptr && env->options()->use_system_ca; | |
| const bool use_system_ca = env != nullptr && env->options()->use_system_ca; | |
| per_process::Debug(DebugCategory::CRYPTO, | |
| "StartLoadingCertificatesOffThread env=%p\n", env); |
| if (tried_cert_loading_off_thread.load() && | ||
| cert_loading_thread_started.load()) { | ||
| uv_thread_join(&cert_loading_thread); | ||
| // Serialize with starters to avoid the race window. |
There was a problem hiding this comment.
I think we now need to move the thread joining code before the freeing code to avoid races.
| per_process::cli_options->per_isolate->per_env->use_system_ca; | ||
| } | ||
| } | ||
| if (ssl_openssl_cert_store) { |
There was a problem hiding this comment.
I think we also now need to add EnvironmentOptions::CheckOptions to validate that --use-openssl-ca and --use-system-ca are mutually exclusive and error out when they are both set. Can you also add a test for it?
| env_options->use_env_proxy = opt_getter("NODE_USE_ENV_PROXY") == "1"; | ||
|
|
||
| #if HAVE_OPENSSL | ||
| env_options->use_system_ca = opt_getter("NODE_USE_SYSTEM_CA") == "1"; |
There was a problem hiding this comment.
Can you add a test for the per-worker env var NODE_USE_SYSTEM_CA? And that --no-use-system-ca overrides it?
3 participants
Makes the
--use-system-caoption a per-environment option rather than a per-process option so that workers can enable/disable them individually